Remove HeaderString::c_str() and migrate callers to getStringView() #6564

dnoe · 2019-04-12T13:22:52Z

Remove the HeaderString::c_str() API, and migrate all callers of it to getStringView() and string_view style usage (ie, absl::string_view::find instead of C style comparisons) wherever appropriate.

Risk Level: Medium. No logic changes intended, but this is delicate and risky code and a large portion of the code base was touched.
Testing: bazel test //test/...
Docs Changes: None
Release Notes: None
Fixes #6494

Signed-off-by: Dan Noé <[email protected]>

This doesn't work, of course. Signed-off-by: Dan Noé <[email protected]>

dnoe · 2019-04-12T14:16:59Z

The old code in the section that failed to build in release and coverage builds is this:

    const std::string& request_override_host =
        downstream_headers->get(Http::Headers::get().EnvoyOriginalDstHost)->value().c_str();

I'll admit to being a little confused about what it means to assign a const char* to a const std::string&. I suppose there's some magic happening under the hood where implicit construction of an rvalue std::string is happening and then the reference is bound to it?

It's also unclear why the other configurations didn't fail to build.

snowp · 2019-04-12T16:11:35Z

@dnoe Yeah it'll create a temporary and its lifetime is extended (https://abseil.io/tips/107)

Signed-off-by: Dan Noé <[email protected]>

Now that we use string_view, we can use range-based-for and make the loop more readable. Signed-off-by: Dan Noé <[email protected]>

lambdai · 2019-04-12T18:03:13Z

Looks great!

lizan

Great, I only reviewed until source/common though.

include/envoy/http/header_map.h

lizan · 2019-04-12T20:34:00Z

source/common/grpc/common.cc

      .inc();
  uint64_t grpc_status_code;
+  std::string grpc_status_string(grpc_status->value().getStringView());


consider use absl::SimpleAtoi instead of atoull (or implement StringUtil::atoull with absl::SimpleAtoi) so we can avoid this string allocation.

Great idea - I will create an issue to follow up by either implementing StringUtil::atoull with absl::SimpleAtoi or converting call sites. I think there was only one spot in the code I found where the return value of StringUtil::atoull (which is C-style) was used.

source/common/grpc/common.cc

mattklein123

Thanks this is absolutely awesome. Huge props for slogging through this and I love seeing the tech debt disappear.

I mainly have comments around adding TODOs for all the other places we can get rid of string copies. We have the opportunity here to speed things up in a non-trivial way as part of these changes which is great.

/wait

include/envoy/http/header_map.h

source/common/access_log/access_log_impl.cc

mattklein123 · 2019-04-12T20:23:07Z

source/common/grpc/common.cc

      .inc();
  uint64_t grpc_status_code;
+  std::string grpc_status_string(grpc_status->value().getStringView());


nit: const here and elsewhere.

Done, though my eyes glaze over reading this diff so please let me know if I missed another :)

source/common/grpc/common.cc

source/common/http/conn_manager_impl.cc

mattklein123 · 2019-04-12T20:27:54Z

source/common/http/header_utility.cc

@@ -87,27 +87,29 @@ bool HeaderUtility::matchHeaders(const Http::HeaderMap& request_headers,
  }

  bool match;
+  absl::string_view header_view = header->value().getStringView();


Do we have to make this copy? If so can we TODO removing it?

This one is just for readability reasons since it gets pretty busy seeing header->value().getStringView() repeated especially when std::regex_match takes begin() and end(). The copy of a string_view should be only slightly worse than a pointer. I've marked it as const, which may prevent the copy entirely.

I'm happy to remove it too if you'd like, but I think it makes this much more readable rather than repeating the header->value().getStringView().

mattklein123 · 2019-04-12T20:33:21Z

source/common/tracing/http_tracer_impl.cc

@@ -66,7 +66,7 @@ Decision HttpTracerUtility::isTracing(const StreamInfo::StreamInfo& stream_info,

  // TODO PERF: Avoid copy.


I think you fixed this?

Indeed I did now that isTraceableUuid takes string_view rather than std::string. Removing the comment, taking the credit (I don't see an existing issue for this, if there is feel free to close it).

source/extensions/filters/http/gzip/gzip_filter.cc

source/extensions/filters/http/lua/lua_filter.cc

Signed-off-by: Dan Noé <[email protected]>

dnoe · 2019-04-15T15:23:18Z

I think everything should be resolved. Follow ups are tracked at #6580 and #6581

mattklein123

Nice this is so awesome. Looking forward to the followup cleanups. A few more comments from another pass. Thank you!

/wait

source/common/http/conn_manager_impl.cc

source/common/http/header_map_impl.h

source/common/http/http1/codec_impl.cc

source/extensions/filters/http/lua/lua_filter.cc

Use lua_pushlstring to avoid std::string construction. Signed-off-by: Dan Noé <[email protected]>

mattklein123

Nice! @lizan do you want to do any additional review?

source/common/http/header_map_impl.cc

Signed-off-by: Dan Noé <[email protected]>

…c_str_by_the_c_shore

lizan

@dnoe can you merge master again?

…c_str_by_the_c_shore Signed-off-by: Dan Noe <[email protected]>

dnoe · 2019-04-16T16:16:16Z

Master merge conflicts addressed.

dnoe added 2 commits April 11, 2019 18:21

Remove HeaderString::c_str and convert user to string_view.

a0bcb92

Signed-off-by: Dan Noé <[email protected]>

Fix up additional c_str() calls found after rebase.

44a20a6

Signed-off-by: Dan Noé <[email protected]>

dnoe requested review from lizan, snowp and zuercher as code owners April 12, 2019 13:22

Don't initialize const string reference from getStringView.

cb604e8

This doesn't work, of course. Signed-off-by: Dan Noé <[email protected]>

mattklein123 self-assigned this Apr 12, 2019

dnoe added 2 commits April 12, 2019 12:41

Follow warning-as-error recommendation from ClangTidy.

a8da8f8

Signed-off-by: Dan Noé <[email protected]>

Use range-based-for loops in TrieLookupTable.

646da68

Now that we use string_view, we can use range-based-for and make the loop more readable. Signed-off-by: Dan Noé <[email protected]>

lizan reviewed Apr 12, 2019

View reviewed changes

mattklein123 requested changes Apr 12, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 12, 2019

dnoe mentioned this pull request Apr 15, 2019

HeaderMap: C string to string_view conversion follow ups #6580

Open

Code review comments & TODOs.

574fa87

Signed-off-by: Dan Noé <[email protected]>

repokitteh-read-only bot removed the waiting label Apr 15, 2019

mattklein123 requested changes Apr 15, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 15, 2019

Check for empty header strings when peeking at first character.

51a0fdb

Use lua_pushlstring to avoid std::string construction. Signed-off-by: Dan Noé <[email protected]>

repokitteh-read-only bot removed the waiting label Apr 15, 2019

dnoe requested a review from mattklein123 April 15, 2019 19:16

mattklein123 previously approved these changes Apr 15, 2019

View reviewed changes

lizan reviewed Apr 16, 2019

View reviewed changes

source/common/http/header_map_impl.cc Outdated Show resolved Hide resolved

Remove C string getExistingInline().

2cf71fe

Signed-off-by: Dan Noé <[email protected]>

dnoe dismissed mattklein123’s stale review via 2cf71fe April 16, 2019 02:10

Merge branch 'master' of https://github.com/envoyproxy/envoy into no_…

9f50523

…c_str_by_the_c_shore

dnoe requested a review from lizan April 16, 2019 13:07

dnoe requested a review from mattklein123 April 16, 2019 13:07

mattklein123 previously approved these changes Apr 16, 2019

View reviewed changes

lizan previously approved these changes Apr 16, 2019

View reviewed changes

Merge branch 'master' of https://github.com/envoyproxy/envoy into no_…

2cdc003

…c_str_by_the_c_shore Signed-off-by: Dan Noe <[email protected]>

dnoe dismissed stale reviews from lizan and mattklein123 via 2cdc003 April 16, 2019 16:15

lizan approved these changes Apr 16, 2019

View reviewed changes

lizan merged commit 3a596a4 into envoyproxy:master Apr 16, 2019

yangminzhu mentioned this pull request Apr 18, 2019

Update envoy sha and fix bulid break istio/proxy#2179

Merged

This was referenced Jun 1, 2019

Build: bazel build failed in network filters #7082

Closed

http integration tests: Migrate c_str to getStringView envoyproxy/envoy-filter-example#87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove HeaderString::c_str() and migrate callers to getStringView() #6564

Remove HeaderString::c_str() and migrate callers to getStringView() #6564

dnoe commented Apr 12, 2019

dnoe commented Apr 12, 2019 •

edited

Loading

snowp commented Apr 12, 2019

lambdai commented Apr 12, 2019

lizan left a comment

lizan Apr 12, 2019

dnoe Apr 15, 2019

mattklein123 left a comment

mattklein123 Apr 12, 2019

dnoe Apr 15, 2019

mattklein123 Apr 12, 2019

dnoe Apr 15, 2019

mattklein123 Apr 12, 2019

dnoe Apr 15, 2019

dnoe commented Apr 15, 2019

mattklein123 left a comment

mattklein123 left a comment

lizan left a comment

dnoe commented Apr 16, 2019

		@@ -66,7 +66,7 @@ Decision HttpTracerUtility::isTracing(const StreamInfo::StreamInfo& stream_info,

		// TODO PERF: Avoid copy.

Remove HeaderString::c_str() and migrate callers to getStringView() #6564

Remove HeaderString::c_str() and migrate callers to getStringView() #6564

Conversation

dnoe commented Apr 12, 2019

dnoe commented Apr 12, 2019 • edited Loading

snowp commented Apr 12, 2019

lambdai commented Apr 12, 2019

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnoe commented Apr 15, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

dnoe commented Apr 16, 2019

dnoe commented Apr 12, 2019 •

edited

Loading